Refactor TimePoint getters and fix decimal quantity dump rounding #187

MetRonnie · 2020-10-16T16:23:20Z

This branch was kicking around for a while; I probably meant to create a PR earlier.

Closes Simplify TimePoint getters #180 - use only @property decorators for getter methods, instead of having a mix of those and a get(self, property_name) method
Closes TimePoint dumping buggy for decimals that round up to 1 #184 - fix bug where decimal quantities were getting rounded up to illegal values when dumping timepoints, e.g. minute of hour = 59.999999999 --> 60.0. Just truncate the quantity instead (in this specific circumstance), so it dumps as 59.999999 to 6 d.p.
Now prevents TimePoint from being init'd without a year (unless it's a truncated TP)

Change log updated
Unit tests included

Fix rounding bug for decimal time quantities

TimePoints are immutable now, so we don't need to worry about testing for mutating an attr to a bad value

metomi/isodatetime/dumpers.py

kinow · 2020-10-16T22:03:11Z

metomi/isodatetime/data.py

@@ -880,11 +880,11 @@ class TimePoint:

    Keyword arguments (usually default to None if not provided):

-    expanded_year_digits (default 0) - an agreed-upon number of extra
-        digits to represent the year, beyond the default of 4. For example,
+    num_expanded_year_digits (default 0) - an agreed-upon number of extra


I don't have a preference for expanded_year_digits or num_expanded_year_digits, but as this property is changing for TimePoint, should we change it in the rest of the code too? grep'ing for expanded.year.digit, looks like test__00.py, parser_spec.py, data.py, parsers.py, dumpers.py, and README.md mention expanded year digits.

Are those reference OK, in which case this rename here is necessary as this property doesn't represent TimePoint's expanded year digits? (not sure if that makes sense, sorry 😆 ), or are we going to rename them too later?

It's the difference between the number of digits and the actual digits themselves

isodatetime/metomi/isodatetime/data.py

Lines 1110 to 1111 in 2b908ad

@property

def num_expanded_year_digits(self): return self._num_expanded_year_digits

isodatetime/metomi/isodatetime/data.py

Lines 1233 to 1237 in 2b908ad

@property

def expanded_year_digits(self):

"""The extra digits at the front of the expanded year, as opposed to

the number of such digits"""

return abs(self._year / 10000)

(not sure why those two are showing different indentations, they're the same)

Before, we had TimePoint.expanded_year_digits which was the number of digits, and TimePoint.get("expanded_year_digits") which was the digits themselves. The former should probably have always been called num_expanded_year_digits, whereas the latter has not been renamed (just converted to @property)

…ound up to 1 This is to prevent e.g. minute_of_hour dumping "60.0". Tests added

Unless truncated TimePoint. Tests included.

MetRonnie · 2020-10-23T16:28:04Z

(The tests on Travis have passed, it's just Codacy complaining about an unassigned variable or something in a test)

oliver-sanders

Looks fine, adds more breaking changes for Cylc to deal with but they are pretty small and easy to handle.

oliver-sanders · 2020-10-26T15:29:48Z

metomi/isodatetime/data.py

+            # Truncate instead of rounding up because ticking over the higher
+            # quantities would be complicated
+            return "999999"
+        string = "%f" % decimal


Perhaps make it clearer that the rounding is happening implicitly in a string formatting expression (I had to look it up):

Suggested change

string = "%f" % decimal

string = "%0.6f" % decimal

Or just use round(decimal, 6)?

oliver-sanders · 2020-10-26T16:47:02Z

metomi/isodatetime/tests/test_00.py

-        value_error_timepoint = data.TimePoint(minute_of_hour=10)
-        value_error_timepoint._minute_of_hour = "1O"
-        self.assertRaises(ValueError, dumper.dump, value_error_timepoint, "%M")


Behaviour change?

Now that TimePoints are "immutable", we shouldn't have to worry about mutating a property to an invalid value

oliver-sanders · 2020-10-26T16:48:57Z

BTW to make codacy happy just dedent that function and remove the unused self arg.

MetRonnie · 2020-10-26T17:59:37Z

Oh, codacy only shows the newest problem it has found with the code. Last time it was failing because of this expression-not-assigned (W0106). Trouble is, if you assign it, flake8 will flag it as an unused variable!

MetRonnie · 2020-11-12T18:31:43Z

@kinow just a reminder this is awaiting your review, no rush though

MetRonnie added 4 commits October 16, 2020 17:11

Change TimePoint's expanded_year_digits to num_expanded_year_digits

d75822c

Change TimePoint properties from get() to @Property

8e147b0

TimePoint: change all remaining get() -> @Property and fix metomi#184

658b31e

Fix rounding bug for decimal time quantities

Remove unnecessary test

9855239

TimePoints are immutable now, so we don't need to worry about testing for mutating an attr to a bad value

MetRonnie added the bug label Oct 16, 2020

MetRonnie added this to the 2.1 milestone Oct 16, 2020

MetRonnie requested review from kinow and oliver-sanders October 16, 2020 16:23

MetRonnie self-assigned this Oct 16, 2020

kinow reviewed Oct 16, 2020

View reviewed changes

MetRonnie added 2 commits October 19, 2020 10:43

TimePoint: Truncate decimal time quantities if they would otherwise r…

66d68bf

…ound up to 1 This is to prevent e.g. minute_of_hour dumping "60.0". Tests added

Raise exception if TimePoint init'd without year

3798759

Unless truncated TimePoint. Tests included.

MetRonnie force-pushed the timepoint-getters branch from 2b908ad to 3798759 Compare October 19, 2020 09:44

Update changelog [skip travis]

c0f9b82

oliver-sanders reviewed Oct 26, 2020

View reviewed changes

oliver-sanders approved these changes Oct 26, 2020

View reviewed changes

Address code review

11695b8

kinow approved these changes Nov 12, 2020

View reviewed changes

kinow merged commit cc9e794 into metomi:master Nov 12, 2020

MetRonnie deleted the timepoint-getters branch November 12, 2020 20:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor TimePoint getters and fix decimal quantity dump rounding #187

Refactor TimePoint getters and fix decimal quantity dump rounding #187

MetRonnie commented Oct 16, 2020 •

edited

Loading

kinow Oct 16, 2020

MetRonnie Oct 19, 2020

MetRonnie commented Oct 23, 2020

oliver-sanders left a comment

oliver-sanders Oct 26, 2020 •

edited

Loading

oliver-sanders Oct 26, 2020

MetRonnie Oct 26, 2020

oliver-sanders commented Oct 26, 2020

MetRonnie commented Oct 26, 2020

MetRonnie commented Nov 12, 2020

	@property
	def num_expanded_year_digits(self): return self._num_expanded_year_digits

	@property
	def expanded_year_digits(self):
	"""The extra digits at the front of the expanded year, as opposed to
	the number of such digits"""
	return abs(self._year / 10000)

Refactor TimePoint getters and fix decimal quantity dump rounding #187

Refactor TimePoint getters and fix decimal quantity dump rounding #187

Conversation

MetRonnie commented Oct 16, 2020 • edited Loading

kinow Oct 16, 2020

Choose a reason for hiding this comment

MetRonnie Oct 19, 2020

Choose a reason for hiding this comment

MetRonnie commented Oct 23, 2020

oliver-sanders left a comment

Choose a reason for hiding this comment

oliver-sanders Oct 26, 2020 • edited Loading

Choose a reason for hiding this comment

oliver-sanders Oct 26, 2020

Choose a reason for hiding this comment

MetRonnie Oct 26, 2020

Choose a reason for hiding this comment

oliver-sanders commented Oct 26, 2020

MetRonnie commented Oct 26, 2020

MetRonnie commented Nov 12, 2020

MetRonnie commented Oct 16, 2020 •

edited

Loading

oliver-sanders Oct 26, 2020 •

edited

Loading